Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 convert binary artifact check to probe #3508

Merged
merged 15 commits into from
Dec 5, 2023

Conversation

AdamKorcz
Copy link
Contributor

What kind of change does this PR introduce?

Feature

What is the new behavior (if this is a feature change)?**

This PR converts the binary artifacts check to a probe.

It does change the processing of requests in checks/raw to align it with the style of other probes.

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

NONE

Does this PR introduce a user-facing change?

NONE


@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Merging #3508 (b333a8f) into main (483cc31) will decrease coverage by 7.62%.
The diff coverage is 75.78%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3508      +/-   ##
==========================================
- Coverage   76.39%   68.78%   -7.62%     
==========================================
  Files         210      212       +2     
  Lines       14421    14496      +75     
==========================================
- Hits        11017     9971    -1046     
- Misses       2757     3946    +1189     
+ Partials      647      579      -68     

checks/evaluation/binary_artifacts.go Outdated Show resolved Hide resolved
checks/evaluation/binary_artifacts_test.go Outdated Show resolved Hide resolved
probes/hasBinaryArtifacts/def.yml Outdated Show resolved Hide resolved
probes/hasBinaryArtifacts/def.yml Outdated Show resolved Hide resolved
probes/hasBinaryArtifacts/impl.go Outdated Show resolved Hide resolved
@laurentsimon
Copy link
Contributor

The check also looks for the installation of maven (or gradle) Action to verify the integrity of wrapper binaries. I think we need more than one probe to handle these cases. Suggestion: hasMavenWrapperBinary, hasMavenWrapperActionInstalled, hasOtherBinaries.

@AdamKorcz AdamKorcz temporarily deployed to gitlab October 3, 2023 13:54 — with GitHub Actions Inactive
@AdamKorcz
Copy link
Contributor Author

The check also looks for the installation of maven (or gradle) Action to verify the integrity of wrapper binaries. I think we need more than one probe to handle these cases. Suggestion: hasMavenWrapperBinary, hasMavenWrapperActionInstalled, hasOtherBinaries.

I only see checks for Gradle Wrappers, however, the filtering happens before the files are added to the BinaryArtifactData. As such, the current Scorecard check + evaluation does not filter out wrapper binaries. Is that something we should change and convert to probes?

@laurentsimon
Copy link
Contributor

The check also looks for the installation of maven (or gradle) Action to verify the integrity of wrapper binaries. I think we need more than one probe to handle these cases. Suggestion: hasMavenWrapperBinary, hasMavenWrapperActionInstalled, hasOtherBinaries.

I only see checks for Gradle Wrappers, however, the filtering happens before the files are added to the BinaryArtifactData. As such, the current Scorecard check + evaluation does not filter out wrapper binaries. Is that something we should change and convert to probes?

yes I think that would be useful. I could image end-users wanted to know if there's a gradle wrapper present in the repo or not. I think it's ok to do this in this. @spencerschrock wdut?

@spencerschrock
Copy link
Member

I only see checks for Gradle Wrappers, however, the filtering happens before the files are added to the BinaryArtifactData. As such, the current Scorecard check + evaluation does not filter out wrapper binaries. Is that something we should change and convert to probes?

yes I think that would be useful. I could image end-users wanted to know if there's a gradle wrapper present in the repo or not. I think it's ok to do this in this. @spencerschrock wdut?

I think it's fine to do the filtering on the evaluation side, but would need to double check against the Allstar code.

The check also looks for the installation of maven (or gradle) Action to verify the integrity of wrapper binaries. I think we need more than one probe to handle these cases. Suggestion: hasMavenWrapperBinary, hasMavenWrapperActionInstalled, hasOtherBinaries.

With regard to probes, I haven't read the PR, but my proposed breakdown was probably going to have multiple probes (although only one would likely be used by the evaluation code):

  • free of binary artifacts
  • free of binary artifacts (except for the "approved" maven ones)

@spencerschrock
Copy link
Member

spencerschrock commented Oct 3, 2023

I think this is another example where it makes sense to clarify what a finding is? If it's some data point which optionally has a file location, then would each binary artifact have its own finding? (note: this is a little different than the repeated contributor/commit findings). If it's gradle verified it could be a gradle validated finding, otherwise a normal binary finding.

And the policy could later walk both, and choose to either count or ignore the gradle verified ones

@AdamKorcz AdamKorcz temporarily deployed to gitlab October 5, 2023 14:54 — with GitHub Actions Inactive
@github-actions
Copy link

Stale pull request message

Copy link

github-actions bot commented Nov 2, 2023

This pull request is stale because it has been open for 10 days with no activity

@spencerschrock
Copy link
Member

With regard to probes, I haven't read the PR, but my proposed breakdown was probably going to have multiple probes (although only one would likely be used by the evaluation code):

  • free of binary artifacts
  • free of binary artifacts (except for the "approved" maven ones)

I believe this is the outcome of our discussion today. @laurentsimon your comment about different logic for each finding, I don't think that's true so long as the policy picks one.

e.g. if you have 3 binaries: foo, bar, and gradle-wrapper.jar (verified)

The first probe would complain about all 3.
the second probe would only complain about foo and bar.

Scorecard evaluation code would use the second probe. The first probe would be dangling/uncalled, until someone writes a policy to use it.

@laurentsimon
Copy link
Contributor

laurentsimon commented Nov 21, 2023

With regard to probes, I haven't read the PR, but my proposed breakdown was probably going to have multiple probes (although only one would likely be used by the evaluation code):

  • free of binary artifacts
  • free of binary artifacts (except for the "approved" maven ones)

I believe this is the outcome of our discussion today. @laurentsimon your comment about different logic for each finding, I don't think that's true so long as the policy picks one.

I think I was wrong too. So long as there's one finding per binary, it should work. So the main benefit of having different probes would be to accommodate different remediations for each "type" of verification. Today I don't know if we need this flexibility. But if we want to not be blocked in the future, that could be beneficial to do it now. I don't see a strong argument for trying to pack multiple verification types into a single "unverifiedProbe". What is the main motivation?

e.g. if you have 3 binaries: foo, bar, and gradle-wrapper.jar (verified)

The first probe would complain about all 3. the second probe would only complain about foo and bar.

I thought we agreed the were mutually exclusive? (can re-visit if it's better not to)

Scorecard evaluation code would use the second probe. The first probe would be dangling/uncalled, until someone writes a policy to use it.

@spencerschrock
Copy link
Member

I thought we agreed the were mutually exclusive? (can re-visit if it's better not to)

I thought the binary type was mutually exclusive, but the probes may not be. Can discuss offline if needed.

@AdamKorcz AdamKorcz force-pushed the binary-artifacts-probe branch from d028684 to 864c68e Compare November 23, 2023 11:27
checks/evaluation/binary_artifacts.go Show resolved Hide resolved
checks/evaluation/binary_artifacts_test.go Outdated Show resolved Hide resolved
checks/evaluation/binary_artifacts_test.go Outdated Show resolved Hide resolved
finding/finding.go Outdated Show resolved Hide resolved
checks/raw/binary_artifact.go Outdated Show resolved Hide resolved
probes/freeOfAnyBinaryArtifacts/impl.go Outdated Show resolved Hide resolved
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
@spencerschrock
Copy link
Member

/scdiff generate Binary-Artifacts

Copy link

github-actions bot commented Dec 5, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move application of Binary Artifacts Gradle Wrapper Action exception into checks/evaluation/
3 participants